-
Notifications
You must be signed in to change notification settings - Fork 412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: clone sig metadata properties #4316
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Put some wondering.
This issue's impacting latest docker images. So we should release a new minor after merging and testing. |
0b164b1
to
73523cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
73523cf
to
a4fbfe3
Compare
- bump sig helpers
a4fbfe3
to
7b32e95
Compare
Forgive me for asking, but are we any better off now than the original version where the function just returned the metadata as a struct? So maybe we have now (at best) (mem footprint)=original-(no copy)+(clone the properties). |
I think you’re right, @NDStrahilevitz. Initially, we saw a performance improvement, but then an issue arose. After discussing it with @geyslan, we’re considering a rollback. Thanks! |
We're already reverted related changes. |
fix: #4298
fix: #4313
1. Explain what the PR does
0b164b1 fix: clone sig metadata properties
2. Explain how to test it
3. Other comments